Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new interface-driven static file server package (staticweb) and comprehensive documentation updates across the ResolveSpec framework.
Key Changes:
- Implements a flexible, router-agnostic static file server with pluggable policies
- Adds support for multiple filesystem backends (local, zip, embedded)
- Includes comprehensive testing utilities and examples
- Updates main README and adds detailed package-specific documentation
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/server/zipfs/zipfs.go | Implements fs.FS interface for zip archives with seek support |
| pkg/server/staticweb/interfaces.go | Defines core interfaces for providers, policies, and service |
| pkg/server/staticweb/service.go | Main service implementation with mount management |
| pkg/server/staticweb/mount.go | Individual mount point HTTP handler implementation |
| pkg/server/staticweb/config.go | Configuration structs and helper constructors |
| pkg/server/staticweb/policies/*.go | Cache, MIME, and fallback strategy implementations |
| pkg/server/staticweb/providers/*.go | Local, zip, and embedded filesystem providers |
| pkg/server/staticweb/testing/mocks.go | Mock implementations for testing |
| pkg/server/staticweb/*_test.go | Comprehensive test suite |
| pkg/server/staticweb/README.md | Complete package documentation |
| README.md | Updated main documentation with staticweb references |
| pkg/restheadspec/README.md | New detailed RestHeadSpec documentation |
| pkg/resolvespec/README.md | New detailed ResolveSpec documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetMIMEType returns the MIME type for the given path. | ||
| func (r *MockMIMEResolver) GetMIMEType(path string) string { | ||
| r.mu.RLock() | ||
| defer r.mu.RUnlock() | ||
|
|
||
| ext := strings.ToLower(path[strings.LastIndex(path, "."):]) | ||
| if mimeType, ok := r.types[ext]; ok { | ||
| return mimeType | ||
| } | ||
| return "application/octet-stream" | ||
| } |
There was a problem hiding this comment.
The GetMIMEType method will panic if the path doesn't contain a dot character, as strings.LastIndex will return -1 and the slice operation on line 185 will be out of bounds. Add a bounds check before slicing, or use strings.LastIndex to check if a dot exists first.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if f.File != nil { | ||
| return f.FileInfo(), nil | ||
| } | ||
| return nil, fmt.Errorf("no file") |
There was a problem hiding this comment.
The error message format "no file" is unclear and unhelpful. Consider providing more context such as "file handle is nil" or "file not properly initialized" to help with debugging.
| return nil, fmt.Errorf("no file") | |
| return nil, &fs.PathError{Op: "stat", Path: f.Name, Err: fmt.Errorf("zip file not initialized: underlying *zip.File is nil")} |
| // If it's a directory, try to serve index.html | ||
| if stat.IsDir() { | ||
| indexPath := path.Join(filePath, "index.html") | ||
| indexFile, err := m.provider.Open(strings.TrimPrefix(indexPath, "/")) | ||
| if err != nil { | ||
| http.Error(w, "Forbidden", http.StatusForbidden) | ||
| return | ||
| } | ||
| defer indexFile.Close() | ||
|
|
||
| indexStat, err := indexFile.Stat() | ||
| if err != nil { | ||
| http.Error(w, "Internal Server Error", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| filePath = indexPath | ||
| stat = indexStat | ||
| file = indexFile | ||
| } |
There was a problem hiding this comment.
The directory serving logic reopens and reads the index file unnecessarily. The file is already open at line 106, stat is retrieved at line 114, but then on lines 120-122 it reassigns variables from the newly opened indexFile. However, the original file handle passed as parameter is not being properly managed. Consider refactoring to either close the original file handle before serving index.html or use the indexFile consistently throughout.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| http.Error(w, "Internal Server Error", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| http.ServeContent(w, r, stat.Name(), stat.ModTime(), strings.NewReader(string(data))) |
There was a problem hiding this comment.
Converting []byte to string and then to strings.NewReader is inefficient. Use bytes.NewReader(data) directly instead of strings.NewReader(string(data)) to avoid an unnecessary allocation and copy.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ## Computed Columns | ||
|
|
||
| Define virtual columns using SQL expressions: | ||
|
|
||
| ```json | ||
| { | ||
| "operation": "read", | ||
| "options": { | ||
| "columns": ["id", "first_name", "last_name"], | ||
| "computedColumns": [ | ||
| { | ||
| "name": "full_name", | ||
| "expression": "CONCAT(first_name, ' ', last_name)" | ||
| }, | ||
| { | ||
| "name": "age_years", | ||
| "expression": "EXTRACT(YEAR FROM AGE(birth_date))" | ||
| } | ||
| ] |
There was a problem hiding this comment.
The computedColumns feature described here allows API clients to send arbitrary SQL expressions in computedColumns[].expression, which are then passed directly into query.ColumnExpr(...) in both resolvespec and restheadspec handlers without sanitization or whitelisting. An attacker who can call these endpoints can craft expressions (including subqueries) that read from any table the database user can access, leading to unauthorized data exposure far beyond the intended model. This should be restricted so clients can only request a fixed set of predefined computed expressions, or the expressions must be strongly validated/whitelisted instead of passing raw SQL strings into ColumnExpr.
|
@copilot See my commits and make a new pull requests for those mentioned |
Added staticweb and a few fixes in the apis.
Better docs.